Skip to content

Bun.serve: close connection when file body is truncated mid-send - #34185

Closed
robobun wants to merge 3 commits into
mainfrom
farm/f29d6eb9/sendfile-truncate-desync
Closed

robobun wants to merge 3 commits into
mainfrom
farm/f29d6eb9/sendfile-truncate-desync

Conversation

@robobun

@robobun robobun commented Jul 15, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Bun.serve serving a Bun.file body: when the backing file is truncated while the response is being sent (log rotation, truncate+rewrite deploy, non-atomic asset rewrite), the server treats early EOF as normal completion. The body ends millions of bytes short of its declared Content-Length with no close, and on a keep-alive connection the next request's entire response (status line + headers + body) is delivered inside the first response's declared body region: an HTTP/1.1 framing desync (RFC 9112 6.2). With idleTimeout: 0 the client stalls forever.

CL=33554432 delivered=2691072 owed=30863360;
  inside owed region: "HTTP/1.1 200 OK\r\nContent-Type: text/plain;..."

Node closes the connection after hitting EOF so the client detects the short body immediately.

Cause

FileResponseStream::on_sendfile treats sent == 0 the same as remain == 0:

if self.sendfile.remain == 0 || sent == 0 {
    self.end_sendfile();   // markDone, leave socket open for keep-alive

sendfile(2) returning 0 with bytes still owed means the input file's EOF is now below our offset. The advertised Content-Length can no longer be honoured, so the only framing-safe recovery is to close the connection. The buffered-reader backend (SSL / macOS / Windows / sub-1MiB files) has the same shape: early ReadState::Eof with max_size > 0 was routed to resp.end().

Fix

On early EOF in either backend, force_close() the socket instead of marking the response done, so the client observes the short body and cannot desync. on_complete still fires for caller cleanup.

Test

test/js/bun/http/bun-serve-file.test.ts gains a Linux-gated pair covering both the static-route path (FileRoute) and the fetch handler path (RequestContext): a 32 MiB file is truncated to 1 KiB mid-stream, the test waits for server.pendingRequests == 0, then asserts the socket closed with a short body and that a follow-up request was not served inside the first body window.


no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-file.test.ts

When the backing file of a Bun.file response shrinks while the body is
being sent (log rotation, truncate+rewrite deploy), sendfile(2) returns
0 with bytes still owed. Previously this was folded into the normal
completion arm, leaving the keep-alive socket open with the response
marked done. The client then waits forever for bytes that will never
arrive (with idleTimeout: 0), and the next request on that socket is
served inside the first response's declared Content-Length window: an
HTTP/1.1 framing desync (RFC 9112 6.2).

Force-close the connection on early EOF so the client observes the
short body immediately, matching Node.js. Applied to both the sendfile
backend (Linux) and the buffered-reader backend (SSL / macOS / Windows /
small files), which share the same shape.
@coderabbitai

coderabbitai Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 34 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 079fc7ef-801a-45e4-a0bd-d8f93e9fc6a9

📥 Commits

Reviewing files that changed from the base of the PR and between 4e5a285 and 0a89fd9.

📒 Files selected for processing (2)
  • src/runtime/server/FileResponseStream.rs
  • test/js/bun/http/bun-serve-file.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:11 PM PT - Jul 14th, 2026

❌ @autofix-ci[bot], your commit 0a89fd9 has 2 failures in Build #73073 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34185

That installs a local version of the PR into your bun-34185 executable, so you can run:

bun-34185 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline coverage nit, I also checked whether the reader-path force_close() dropping the final chunk (which the old resp.end(chunk, …) would have written) is a problem — it isn't: the body is already short of Content-Length, so the client must treat it as truncated regardless of whether that last partial chunk lands before the close.

Extended reasoning...

The finder agents flagged that on the reader-backend short-EOF path, self.resp.force_close() replaces self.resp.end(chunk, …) and therefore never writes the final chunk bytes. Verifiers confirmed this is intentional and harmless: the connection is being torn down precisely because the advertised Content-Length can no longer be honoured, so whether the client receives N or N+chunk.len() bytes before the close, it observes the same truncated-body condition. The fail_with() path already uses the identical detach_resp(); force_close(); (on_…)(ctx, resp) sequence, so lifecycle ordering is established. Not approving because this is core Bun.serve connection-lifecycle code and CI hasn't reported yet.

Comment thread test/js/bun/http/bun-serve-file.test.ts Resolved
robobun and others added 2 commits July 15, 2026 00:24
The buffered-reader path (SSL / macOS / Windows) reaches early EOF via
on_reader_done -> finish(), not on_read_chunk(_, Eof), when the resumed
read() returns 0 with an empty stack buffer. Move the short-body check
into finish() so that path force-closes too, and route on_read_chunk's
short-EOF case through the same path.

Add SSL test variants (static route + fetch handler) that exercise the
reader backend on all platforms; the plain-TCP variants remain
Linux-gated to exercise sendfile.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — 37cfce2 fully addresses the reader-backend coverage gap (and turned up the on_reader_done → finish() path I'd missed). No further issues found, but this touches Bun.serve connection lifecycle so worth a human look before merge.

Checked: end_sendfile(short) / finish() follow the existing fail_with() pattern (detach_resp() before force_close(), on_complete still fires); max_size.is_some_and(|m| m > 0) in finish() is only reachable via on_reader_done/on_writable with !RESPONSE_DONE, so it can't force-close a fully-delivered body (max_size is decremented to 0 on the happy path) or a pipe/FIFO (length: None). The 4-variant test now covers sendfile × reader backend × FileRoute × RequestContext.

Extended reasoning...

Overview

The PR changes early-EOF handling in FileResponseStream (the shared file-body streamer for Bun.serve static routes and fetch-handler Bun.file responses). Three sites now detect "file shrank below advertised Content-Length" and force_close() the socket instead of ending the response cleanly on a keep-alive connection: on_sendfile (Linux sendfile backend, sent == 0 && remain > 0), on_read_chunk (defensive early-return on Eof with max_size > 0), and finish() (the actual reader-backend termination point). A new 4-variant test in bun-serve-file.test.ts truncates a 32 MiB file mid-stream and asserts the socket closes short rather than desyncing a follow-up pipelined request.

Changes since my last review

My previous comment flagged that the reader backend had no test coverage. The author added SSL variants (which force the reader backend on all platforms via can_sendfile() returning false for non-TCP) and, in doing so, discovered the original on_read_chunk fix was dead code — early EOF on a resumed zero-byte read reaches finish() via on_reader_done, not on_read_chunk(_, Eof). The fix was moved to finish() and the on_read_chunk guard kept as a defensive early-return. All four test variants are reported to fail on stock bun and pass with the fix.

Security risks

None introduced. The change tightens behavior (closing a connection that would otherwise desync) and does not add any new input parsing or trust boundary.

Level of scrutiny

Medium-high. This is production Bun.serve connection-lifecycle code interacting with uWS callback userdata and intrusive refcounts. The changes themselves are small and follow the existing fail_with() pattern exactly (detach_resp() → force_close() → callback), and I traced that finish()'s new max_size > 0 check cannot fire on the happy path (decremented to Some(0)) or for unbounded streams (None). But subtle uWS lifetime interactions in this file have caused UAFs before (see the neighboring FIFO-backpressure and double-close regression tests), so a maintainer familiar with AnyResponse::force_close semantics should confirm.

Other factors

CI build #73073 was still running at review time and the PR description notes the tests were not run locally (platform-gated). The plain-TCP variants remain skipIf(!isLinux), which is reasonable given sendfile is Linux-only, and the SSL variants run everywhere.

@robobun

robobun commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

CI build #73073: 283/286 passed. bun-serve-file.test.ts (with the four new truncation variants) passed on all lanes including darwin, Windows, and alpine/musl.

The three red lanes failed on tests that also fail on main and are unrelated to this diff:

  • test-worker-message-port-transfer-terminate.js (JSC assertion, debian x64-asan)
  • test-net-connect-memleak.js (alpine x64 / x64-baseline)

Ready for review.

Jarred-Sumner pushed a commit that referenced this pull request Aug 16, 2026
…after it (#39296)

Follow-up to #39201, where moving the slice window into the reader was
agreed as a separate PR.

### Problem
- Streaming a slice of a pipe, socket or stdin consumes bytes past the
slice. With `0123456789` written to a child's stdin, `for await (const c
of Bun.stdin.slice(0, 3).stream())` delivers `012` and whoever reads
stdin next gets nothing: the other 7 bytes were read and dropped. Same
through a native sink (`HTMLRewriter.transform(new
Response(Bun.stdin.slice(0, 3)))`). With 60000 bytes written, the stream
leaves 0 of them in the pipe. Same on Windows.
- `Bun.stdin.slice(0, 0).stream()` (any empty slice of a pollable
source) never ends while the pipe stays open and silent; it ends only
when a byte arrives, which it drops, or at EOF. Same through a native
sink.
- `Bun.serve` responding with `Bun.file(p).slice(0, 2048)` of a 4 KiB
file issues two `read()`s per response (a 256 KiB request that returns
the whole file, then an EOF probe) and ends the response from a queued
task. Harmless for regular files, but it is the same after-the-fact
window as above, implemented a second time.
- Cause: the window is applied by the reader's parents after the bytes
have been read. `FileReader::on_read_chunk` cuts the delivered chunk
down (`src/runtime/webcore/FileReader.rs:636`); the only place that cut
the read itself was `on_pull`'s `read_into` branch
(`FileReader.rs:849`), which is skipped whenever a poll is registered,
i.e. for every pollable fd, and `read_into` is a stub on Windows. The
read loop in `src/io/PipeReader.rs` reads up to its 256 KiB scratch
buffer (`fill_scratch`, line 653) and the Windows reader hands libuv its
whole spare capacity (`get_read_buffer_with_stable_memory_address`, line
1166); neither knows about the window. The empty-window hang is the same
gap: the check lived on the `read_into` branch, and a pollable source
parks on its poll before reaching it. `FileResponseStream` keeps a third
copy of the window (`src/runtime/server/FileResponseStream.rs:277`) and
needs the `FileResponseStreamEof` task to end itself.

### Fix
- `PosixBufferedReader` and `WindowsBufferedReader` get a `ReadLimit`
next to the pread offset, set with `set_limit` before starting.
`read_once` (every POSIX read: loop, buffer and `read_into`) and the
libuv buffer on Windows cut each read to what is left of it, and
charging it down to zero is reported the way EOF already is (`Stop::Eof`
/ `ReadState::Eof`, then `close` and `on_reader_done`). A limit that
starts at zero makes `start` register no poll and issue no read; the
parent's first read request (`read`, `read_into`, Windows `unpause`)
reports EOF without touching the fd.
- `FileReader` passes the blob's size to the reader at start and
`FileResponseStream` passes the body length; both now treat the end of
the window exactly like EOF, because that is what the reader reports.
Deleted: `FileReader`'s `total_readed` / `window_remaining` /
`consume_window` / `end_at_window`, `FileResponseStream`'s `max_size`
and the `FileResponseStreamEof` task (tag, dispatch arm, `Taskable`
impl), and `Chunk::truncate`, which only existed for this.
- `FileResponseStream` implements the reader's `ref_` / `deref` (as
`FileReader` does). The chunk that fills the window is now followed by
`on_reader_done`, which releases the stream's owning ref, while the
reader's read loop (POSIX) or libuv completion (Windows, which re-checks
the reader's flags after delivering a data read) is still on the stack
with the reader embedded in the stream; the pin keeps the stream alive
until those frames return. This is the job the queued task used to do.
- Why this is the right place: the window is a bound on how many bytes
may be taken out of the source, and on anything that is not seekable the
bytes a read returns are gone, so the bound has to be applied to the
read, and only the reader knows whether a read is needed at all.
Reporting it as EOF is correct because for this reader the source does
end there, and every consumer already handles EOF (real EOF and
`maxBuffer` exhaustion end streams through the same path), which is what
lets the two parent implementations (three code paths, two of which
#39201 had to bring back into agreement) go away. For regular files the
delivered bytes are unchanged; only the reads get shorter.
- Verified with `bun bd test test/js/bun/util/bun-stdin-slice.test.ts
test/js/bun/http/serve-file-slice-read-error.test.ts`. The five new
`bun-stdin-slice` cases run the slice in a grandchild sharing the
child's stdin and then read what is left: a write larger than the window
and an empty window with nothing written, each via `stream()` and via
HTMLRewriter, plus a window filled by a write larger than its remainder.
With `src/` stashed, the three over-read cases get `""` instead of
`3456789` / `456` back and the two empty-window cases time out; with the
fix all pass on Linux and on a Windows x64 debug build (the block is not
Windows-skipped; the same protocol reproduces the over-read with the
current Windows canary). `serve-file-slice-read-error.test.ts` kept its
ptrace read counter: 3 responses now take 3 `read()`s and the EIO
injected into the second one fails that response alone (`DONE 2`);
without the fix it is 6 reads and `DONE 3`.
- Also run with the fix: `blob.test.ts` (the #39201 window cases:
`read_into`, sink, Windows `on_read_chunk`, empty window on a regular
file), `bun-serve-file`, `bun-serve-static`, `serve-directory-routes`
(empty files give `FileResponseStream` a zero length), `bun-file*`,
`spawn.test.ts`, `spawn-maxbuf` (the budget shares `read_once` /
`on_read`), `spawn-streaming-stdout`, `spawn-stream-serve`,
`spawn-stdout-filereader-gc-uaf`, `streams.test.js`, `html-rewriter*`,
`body*.test.ts`, `process-stdin`, `bun-write`; on Windows additionally
`blob`, `bun-serve-file`, `bun-serve-static`, `spawn-maxbuf`,
`bun-file*`. `cargo fmt` clean.

### Background
- `BufferedReader` (`src/io/PipeReader.rs`, one POSIX and one libuv
implementation behind a cfg alias) reads an fd on behalf of a parent
embedded around it and reports through a vtable: `on_read_chunk` per
read, then `on_reader_done` at EOF or `on_reader_error`. `FileReader` is
the parent behind a file-backed `ReadableStream` (`Bun.file().stream()`,
`new Response(file).body`, the stream a native sink such as HTMLRewriter
or a fetch body pulls from); `FileResponseStream` is the parent that
writes a `Bun.serve` file body to the socket.
- A file blob carries an offset and a size; for a slice these describe
the window. `ReadableStream::from_blob_copy_ref` copies them into
`FileReader` as `start_offset` / `max_size`; `Bun.serve` computes the
body's offset and length from the blob (or a Range header) and passes
them to `FileResponseStream`. The offset was already the reader's job
(`start_file_offset`); this PR gives it the length as well.
- On POSIX a pollable fd (pipe, socket, tty) is read when its poll
fires, from the reader's own loop; a regular file is read synchronously
when the parent asks (`read`, or `read_into` straight into the JS pull
buffer). `FileReader::on_pull` only uses `read_into` when no poll is
registered, which is why a window applied there never affected pollable
sources. On Windows every read completes through libuv into a buffer the
reader allocates up front, so that allocation is where a read's size is
decided.
- `ref_` / `deref` on the parent vtable are taken by the reader around a
read dispatch (`ParentKeepAlive`) because the reader touches its own
fields after calling into the parent; a parent whose `on_reader_done`
can free it needs them so that free happens after the dispatch returns.
`maxBuffer` (`MaxBuf`) is the reader's other byte bound; the limit is
charged and reported alongside it.

<details>
<summary>Related open PRs</summary>

- #39257 (alii) fixes the empty-window hang inside `FileReader` with the
helpers this PR deletes; if it lands first this PR will be rebased to
drop those two guards (the reader covers them) and keep its tests, which
pass here as well. The over-read is not covered there.
- #31680 and #33510 are about the buffered (`.text()` /
`.arrayBuffer()`) path in `blob/read_file.rs`, which does not use
`BufferedReader`; #31680's `FileReader.rs` hunk was made unnecessary by
#39201.
- #34185 wanted `FileResponseStream`'s remaining count to tell a short
file from a complete one; with this PR that is the bytes it has written
versus the length it was given.
</details>
@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: superseded by #41622. That change covers the same bug with one shared byte balance across the sendfile and reader backends, adds the finish() EOF site that this branch misses, and tests both plain and TLS. This branch is in conflict with main on both files.

@robobun robobun closed this Sep 6, 2026
@robobun

robobun commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator Author

Correction to my closing comment above. This branch does handle the finish() end (the reader ends with no data left), and its tests cover plain TCP and TLS. I was wrong on both points.

The reasons to continue in #41622 are these:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant